added Enable/Disable WiFi functionality #4670
Conversation
* added isEnabled() to Usermod base class * standardized mod_id parameter in UsermodManager (some were uint8_t, some were uint16_t)
disable wifi
WalkthroughThis change introduces a new method, Changes
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/wled.cpp (1)
569-841: Consider adding explicit WiFi shutdown when disabled.While the current implementation correctly prevents WiFi operations when WiFi is disabled, it might be beneficial to explicitly turn off the WiFi hardware when
wifiEnabledis false to save power.Currently, the code skips operations but doesn't actively shut down WiFi if it was previously enabled before the flag was changed.
You could add code like this near the beginning of
handleConnection():void WLED::handleConnection() { static bool scanDone = true; static byte stacO = 0; const unsigned long now = millis(); const unsigned long nowS = now/1000; const bool wifiConfigured = WLED_WIFI_CONFIGURED; + // Ensure WiFi is completely turned off when disabled + if (!wifiEnabled && (WiFi.getMode() != WIFI_OFF)) { + DEBUG_PRINTLN(F("WiFi disabled, turning off radio")); + WiFi.mode(WIFI_OFF); + return; + } // ignore connection handling if WiFi is configured and scan still running // or within first 2s if WiFi is not configured or AP is always active if ((wifiConfigured && multiWiFi.size() > 1 && WiFi.scanComplete() < 0) || (now < 2000 && (!wifiConfigured || apBehavior == AP_BEHAVIOR_ALWAYS))) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
package.json(1 hunks)wled00/button.cpp(1 hunks)wled00/cfg.cpp(2 hunks)wled00/const.h(1 hunks)wled00/data/settings_wifi.htm(2 hunks)wled00/fcn_declare.h(3 hunks)wled00/json.cpp(2 hunks)wled00/network.cpp(1 hunks)wled00/set.cpp(1 hunks)wled00/um_manager.cpp(1 hunks)wled00/wled.cpp(8 hunks)wled00/wled.h(2 hunks)wled00/xml.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
wled00/um_manager.cpp (2)
wled00/fcn_declare.h (4)
getUMData(454-454)data(419-419)data(419-419)isModEnabled(474-474)usermods/audioreactive/audio_reactive.cpp (2)
data(1438-1443)data(1438-1438)
wled00/wled.cpp (3)
wled00/wled.h (1)
resetAP(1068-1068)wled00/util.cpp (2)
prepareHostname(118-139)prepareHostname(118-118)wled00/udp.cpp (4)
espNowSentCB(945-947)espNowSentCB(945-945)espNowReceiveCB(950-1033)espNowReceiveCB(950-950)
wled00/fcn_declare.h (1)
usermods/mpu6050_imu/usermod_gyro_surge.h (1)
um_data(163-197)
🔇 Additional comments (34)
package.json (2)
28-29: Reordering dependencies in package.json.The order of dependencies in package.json doesn't affect functionality. This change simply reorders "nodemon" and "web-resource-inliner" alphabetically.
34-35: Added trailing newline.Adding a trailing newline at the end of JSON files is a standard best practice.
wled00/const.h (1)
213-213: Added new usermod ID for WiFi enable/disable feature.The new usermod ID is properly defined with a sequential ID (58) and references the implementation file "disable_wifi.h". This aligns with the PR objective of adding WiFi enable/disable functionality.
wled00/set.cpp (1)
23-23: Added WiFi enablement flag processing.This line captures the WiFi enabled state from the HTTP request parameter "EN" when handling WiFi settings updates. It follows the same pattern used for other WiFi settings in this function.
wled00/xml.cpp (2)
201-201: Added WiFi enabled checkbox to settings form.Properly adds the checkbox for enabling/disabling WiFi to the settings page, using the same pattern as other settings.
202-206: Added dynamic UI control for WiFi-related elements.These lines dynamically enable or disable WiFi-related UI controls based on the WiFi enabled status. This provides appropriate UI feedback when WiFi is disabled. The implementation targets:
- The scan button
- SSID input field
- Password input field
- BSSID input field
This ensures users can't attempt to configure WiFi settings when the feature is disabled.
wled00/cfg.cpp (2)
100-100: WiFi enabled flag integration in config deserializationThe new code correctly adds support for loading the wifiEnabled flag from configuration JSON. The bitwise OR operation (
|) preserves the current value if the key is missing in the JSON.
769-769: WiFi enabled flag integration in config serializationThis change properly adds the wifiEnabled flag to the WiFi configuration JSON object during serialization, completing the configuration persistence feature.
wled00/network.cpp (1)
322-322: Properly integrated WiFi enabled check in configuration verificationThe modification correctly adds the WiFi enabled state as a prerequisite for considering WiFi configured. This ensures WiFi operations won't be attempted when disabled.
wled00/data/settings_wifi.htm (2)
159-162: Added WiFi enable/disable UI control with restart noticeGood addition of the WiFi enable checkbox with a clear message that the change requires a device restart.
108-116: Well-implemented UI state management for WiFi controlsThe function effectively toggles the disabled state of relevant WiFi configuration controls based on the checkbox state. This is a good UX improvement that prevents users from configuring WiFi while it's disabled.
wled00/um_manager.cpp (3)
34-34: Expanded usermod ID range by changing parameter typeChanging
uint8_ttouint16_tformod_idexpands the range of supported usermod IDs from 256 to 65536, providing room for future growth.
36-36: Consistency improvement for mod_id comparisonUpdated comparison logic to match the wider
uint16_ttype. This change maintains consistency with the parameter type change.
41-47: New method to check if a usermod is enabledThis method properly implements checking if a specified usermod is enabled, which is essential for the WiFi enable/disable functionality. The implementation correctly returns the usermod's enabled state when found.
However, there's a potential issue: if the requested usermod is not found, the function returns false. Consider adding a comment to clarify this behavior or returning a more explicit result like
-1to distinguish between "not found" and "found but disabled".wled00/json.cpp (2)
480-486: Thorough implementation of WiFi-dependent AP mode logic.The change correctly integrates the new
wifiEnabledflag with AP mode activation logic. AP mode will only be started if currently inactive, explicitly requested, and WiFi is enabled. Similarly, AP mode will be stopped if it's active but either WiFi is disabled or AP mode is no longer requested.
727-727: Good addition of WiFi enabled state to JSON information.Adding the WiFi enabled state to the serialized info JSON output is appropriate and necessary for clients to properly reflect the current WiFi state.
wled00/fcn_declare.h (3)
433-433: Good default implementation for isEnabled() method.Adding this virtual method to the
Usermodbase class provides a clean extension point for implementing enable/disable functionality in usermod classes. The default implementation returningtrueensures backward compatibility with existing usermods.
454-454: Parameter type change for usermod ID.Changing the
mod_idparameter fromuint8_ttouint16_tallows for a larger range of usermod IDs. The comment correctly explains thatUSERMOD_ID_RESERVEDwill poll all usermods.
474-474: New function to check if a usermod is enabled.This function provides a way to check if a specific usermod is enabled, which is essential for the new WiFi enable/disable functionality.
wled00/wled.h (5)
348-348: Good addition of WiFi enable flag to WiFiOptions structure.The 1-bit flag for WiFi enabled state is memory-efficient and properly integrated into the WiFiOptions structure.
350-358: Updated constructor to initialize new WiFi enabled flag.The constructor properly initializes the new
wifiEnabledflag.
361-363: Consistent initialization of wifiEnabled to true by default.The flag is initialized to
truefor both ESP32 and other architectures, which is a good backward-compatible default.
371-371: Added access macro for the WiFi enabled flag.This macro provides a consistent way to access the WiFi enabled flag regardless of the underlying implementation.
383-383: Added global wifiEnabled variable for non-RAM-saving configurations.This complements the packed structure approach for configurations without
WLED_SAVE_RAM.wled00/button.cpp (2)
325-346: Changed button handling logic for button 0.The restructured code now has a clearer logic flow with nested if-else blocks:
Long press > WLED_LONG_AP (5 seconds):
- If press > WLED_LONG_FACTORY_RESET (10 seconds): performs factory reset
- Otherwise: starts AP mode with
initAP(true)Short press ≤ WLED_LONG_AP: sets
doReboot = trueThe major change here is that short presses now trigger a reboot rather than the previous behavior. This seems intentional as it fits with the PR description of long press to start AP mode.
Please confirm that the change in behavior for short press on button 0 (now triggering a reboot) is intentional.
334-341: Commented usermod code for WiFi management.There's commented code that seems to be related to enabling WiFi through a usermod. Since it's commented out, it has no functional impact, but it provides useful context about potential future integration with a dedicated WiFi management usermod.
wled00/wled.cpp (8)
569-570: Good implementation of WiFi enablement check in initAP().The code properly prevents Access Point initialization when WiFi is disabled or when AP behavior is set to button-only mode, which aligns with the PR's objective to control WiFi functionality.
572-574: Enhanced debugging for AP behavior.Adding debug output for the current AP behavior setting is helpful for troubleshooting WiFi connectivity issues.
637-639: Well-placed early return for disabled WiFi.This early return properly prevents all WiFi connection attempts when WiFi is disabled, ensuring the device respects the user's preference to disable WiFi functionality.
674-691: Properly gated ESP-NOW initialization.The ESP-NOW initialization is now correctly only performed when WiFi is enabled, while maintaining all the original functionality. The indentation changes make the code more readable.
765-765: Correct WiFi enablement check in handleConnection().Adding the WiFi enabled condition here prevents unnecessary reconnection attempts when WiFi is deliberately disabled.
796-796: Properly gated disconnection handling.This check ensures the system doesn't try to handle WiFi disconnections when WiFi is intentionally disabled, which is the correct behavior.
823-825: Improved debug information for AP startup.The enhanced debug message now includes the AP behavior setting, which is helpful for troubleshooting AP initialization issues.
841-841: Properly conditioned improv response handling.The code correctly prevents sending improv responses when WiFi is disabled, ensuring consistent behavior when WiFi functionality is turned off.
|
One thing that I forgot to note... I do not have an Ethernet board to test the USER_ETHERNET features. I think that I covered that area correctly, however, if someone with a board could test that Ethernet still works correct, I would greatly appreciate it. |
|
I'd also be interested in others' thoughts regarding the "turn WiFi off" comments from code rabbit. |
There was a problem hiding this comment.
I'm afraid I'll have to say no to this PR.
First, if you disable WiFi, how do you enable it again? Even AP mode on button press is disabled so there will be no way to enable it again. Or did I miss such behaviour?
Second, even though ESP-NOW uses WiFi circuitry it is not a WiFi protocol and should not be disabled. There is an option for enabling/disabling ESP-NOW in the settings.
Thirdly, there are unnecessary changes.
the AP still starts with a button0 longpress (see 1st line in initAP().
I did not realize that ESP-NOW was not a WiFi protocol. I'll remove those code changes. That was my mistake.
|
|
ESPNow and wifi hardware are tightly entangled, it is not possible to disable wifi and have ESPNow enabled, at least in arduino framework afaik. |
The setting initializes to Active, can be enabled via b0 longpress, ETH (if installed), the BLE usermod (once I finish it) OK, @blazoncek said in an earlier PR of mine that he felt that disable WiFi should be integrated, not a usermod. So, I'm not sure which direction to go there... I already ripped out the usermod code. |
|
I've made the code changes noted above. But, I still have to test the rebuild. I'll do that tonight and refresh the PR. |
|
Added JSON API
|
I'm trying to track down the cause, but I've found, what I think is, an issue. if, however, (side note, it appears that saving a preset with "apply at boot" causes that preset to execute immediately. is that a bug?) |
figured it out... |
|
Does it need to release IP? It will get a new one on reconnect if needed. As for boot preset: It is executed in async fashion (as are all presets). |
I, personally, don't care as long as the radio is off. It feels like bad practice, though... I'm not sure. I'll spit out some more debug statements and do some more testing after work. I'll pull in my BLE code and verify it doesn't crash. LOL
yeah, I figured. as is the WiFi connect call, I'm seeing. Hence the timing of it all. I'll try to get it to behave with a little more consistency. |
|
Async behaviour is intended. It should be implemented everywhere. |
|
I verified that the connection status is reporting as connect in the situation discussed earlier... I've updated the code to handle it. |
|
It maybe a stupid question, but why do you want to disable WIFI? |
|
additional testing showed that AP was not being enabled/disabled properly.... fixed now |
it's actually mentioned in many of the comments of this PR.
|
|
any decisions on this @blazoncek ? @DedeHai ? |
|
can you explain the current state? |
I know this may work for you but I am not comfortable with the implementation as it is based on a workaround. I am not sure why would you want to circumvent initial WiFi connect as it has no side effects and will run at most for about 10 or so seconds. What I particularly don't like is disconnecting WiFi in event handling. IMO it is not the correct place to do it. Unfortunately I'm lacking in time ATM for testing but have a slightly different approach in my dev branch. Once I test it I'll let you know if it is any better. |
great question for me, personally, I don't want to turn off the circuitry, bc I'm trying to use it for BLE, so I saw the benefit for both options. |
My testing showed that the actual timing was not actually turning off the WiFi if it is configured when using a preset to run on boot. I placed a gazillion log statements in so that I could trace it out and it played out in this order:
if there is a better way to have the preset run on boot, but delay until after WiFi connects, that would be perfect I think I thought about adding a
agreed. I wasn't too happy about that, either. however, my BLE startup kept crashing. :)
I haven't had a chance to look at your fork yet. I try to get to that this weekend. If you have reworked the WiFi startup though, it'll probably work a lot smoother. Please keep me in the loop. I really would love for my contributions to be of value and not just a "well it works for me so I'll just run my own builds" scenario. So, I greatly appreciate all the feedback. |
|
@rhkean what I meant with "why would you want to circumvent initial WiFi connect" is: let it connect and turn off WiFi after about 10 or so seconds using a playlist with two preset - one dummy that does nothing but runs for 10 (or so) seconds and another one that turns off WiFi. |
well, obviously, I didn't think of that. ROFL here's the simplified implementation. Only a I noticed that the AP shutdown code was implemented 6 different times, so I consolidated it into a single WLED method. |
|
Hey folks. I just wanted to chime in here as a user who would love to turn off wifi (temporarily). My use case is battery power applications, where I have WLED running on a power bank. I don't need wifi to be on the entire time, but having it on for a little while when I plug it in is good. I found the "AP opens: Temporary (no connection after boot)" setting and thought that would be what I wanted, but it turns out to only disable broadcasting, not actually turning off the wifi hardware. I still leave it on because I don't need people connecting to my WLED controller and I can always unplug it if I need to change settings. Perhaps that could be another way that this change could be used, specifically helpful for battery-powered applications. If the setting actually saved power, that would be amazing! It may also be wise to mention that it's only on for 5 minutes in the setting description somewhere, as it's not very clear what "no connection after boot" actually means in practice. |
if you have a Button0 (Button connected to GPIO-0), try the Never setting. In code, it's actually called "Button only". It will not turn on the AP unless you hold the button for 5-6 seconds. |
You should've checked my fork as suggested.
Beware, holding it too long and it will be factory reset. |
so... removed the shutdownAP method? close the PR? |
|
Nothing, I'm just pointing out it clashes with what I've done some 9 months ago, yet I'm still not comfortable with my solution too. |
ah... that makes sense. thanks (side note: I've managed to get AP and BLE working together. STA and BLE still crashes, though... probably an advertising interval issue or something) |
That's good to know and I'll try it out for devices that have a button. Some of mine don't, so I'll need to find another solution like fixing the temporary AP shutdown. I still think it is a good idea that should happen either in this or a follow-up PR once the shutdown code is consolidated. |
Summary by CodeRabbit
New Features
Bug Fixes